Fix geizhals price parsing by JulianKahnert · Pull Request #15990 · home-assistant/core

JulianKahnert · 2018-08-15T18:20:35Z

Description:

BREAKING CHANGE: The configuration changes from

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985
    description: "Bose QC35"
    domain: 'geizhals.de'
    regex: '\D\s(\d*)[\,|\.](\d*)'

to:

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985
    description: "Bose QC35"
    locale: "DE"

Possible locale values are: AT, EU, DE, UK or PL

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#6026

Example entry for `configuration.yaml` (if applicable):

sensor:
  - platform: geizhals
    name: qc35
    product_id: 1696985

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code communicates with devices, web services, or third-party tools:

New dependencies have been added to the REQUIREMENTS variable (example).
New dependencies are only imported inside functions that use them (example).
New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
New files were added to .coveragerc.

MartinHjelmare

The GeizParser class should be broken out into standalone library published on pypi.

https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#6-communication-with-devices-services

balloob · 2018-08-16T11:07:09Z

homeassistant/components/sensor/geizhals.py

-        raw = soup.find_all('span', attrs={'itemprop': 'name'})
-        self.device_name = raw[1].string
+        # Parse name
+        raw = soup.find('h1', attrs={'class': 'gh-headline'})


This should be extracted from Home Assistant and into a separate Python library. We will not be able to accept any changes to this platform until this is fixed.

Thanks for your comments @MartinHjelmare and @balloob ! I will create a separate library for that.

JulianKahnert · 2018-08-17T04:40:03Z

I found a bug in the geizhals Library. Please do not merge it yet!

MartinHjelmare · 2018-08-17T05:18:03Z

Before we merge we should also add a paragraph in the PR description about the breaking change, since config options changed.

JulianKahnert · 2018-08-17T17:36:16Z

The Geizhals library was fixed in JulianKahnert/geizhals@5401a94 and the HA component was fixed in b949fc5.

@MartinHjelmare, I added a description of the breaking change in the PR description.

MartinHjelmare

We should initialize self.device in init.

JulianKahnert · 2018-08-17T21:10:04Z

Thanks @MartinHjelmare for your reply. You are absolutly right! I added this in 828d556

JulianKahnert · 2018-08-22T16:20:53Z

Hey @MartinHjelmare , can you merge this?

MartinHjelmare · 2018-08-22T16:28:21Z

There seems to be a problem installing the geizhals library. The travis job failed:
https://travis-ci.org/home-assistant/home-assistant/jobs/417445670#L727

It looks like the package is imported in setup.py. That won't work since dependencies aren't installed at that time.
https://github.com/JulianKahnert/geizhals/blob/master/setup.py#L2

MartinHjelmare · 2018-08-23T15:14:17Z

homeassistant/components/sensor/geizhals.py

-        self.data = GeizParser(product_id, domain, regex)
-        self._state = None
+        self.product_id = product_id
+        self.device = Device()


Please use another attribute name. Entity device is a property now after a recent PR.

balloob · 2018-08-24T12:45:08Z

You still have an installation issue with your lib. You cannot import your own lib inside setup.py because it needs to read setup.py to see what your dependencies are and install them.

JulianKahnert · 2018-08-24T17:56:10Z

Hi there, I'm sorry for wasting your time! d20403d should fix the problem.

houndci-bot · 2018-08-24T17:59:19Z

homeassistant/components/sensor/geizhals.py

undefined name 'add_devices'

fix geizhals price parsing

acae9cc

homeassistant added platform: sensor.geizhals small-pr PRs with less than 30 lines. cla-signed labels Aug 15, 2018

ghost added the in progress label Aug 15, 2018

Fix lint issue

e2ffe42

homeassistant added the cla-signed label Aug 16, 2018

ghost assigned fabaff Aug 16, 2018

MartinHjelmare requested changes Aug 16, 2018

View reviewed changes

balloob requested changes Aug 16, 2018

View reviewed changes

JulianKahnert changed the title ~~fix geizhals price parsing~~ WIP: fix geizhals price parsing Aug 16, 2018

switch to the geizhals pypi package

ab7986a

homeassistant added the cla-signed label Aug 16, 2018

JulianKahnert mentioned this pull request Aug 16, 2018

update geizhals doc home-assistant/home-assistant.io#6026

Merged

2 tasks

JulianKahnert changed the title ~~WIP: fix geizhals price parsing~~ fix geizhals price parsing Aug 16, 2018

throttle updates

eda5c1b

homeassistant added the cla-signed label Aug 16, 2018

balloob approved these changes Aug 16, 2018

View reviewed changes

MartinHjelmare changed the title ~~fix geizhals price parsing~~ WIP: Fix geizhals price parsing Aug 17, 2018

MartinHjelmare added the breaking-change label Aug 17, 2018

update geizhals version

b949fc5

homeassistant added the cla-signed label Aug 17, 2018

JulianKahnert changed the title ~~WIP: Fix geizhals price parsing~~ Fix geizhals price parsing Aug 17, 2018

MartinHjelmare reviewed Aug 17, 2018

View reviewed changes

initialize empty device

828d556

homeassistant added the cla-signed label Aug 17, 2018

minor changes to trigger another TravisCI test

86b7b34

homeassistant added the cla-signed label Aug 23, 2018

MartinHjelmare requested changes Aug 23, 2018

View reviewed changes

device => _device

556a35e

houndci-bot reviewed Aug 24, 2018

View reviewed changes

homeassistant/components/sensor/geizhals.py Outdated

Copy link
Copy Markdown

houndci-bot Aug 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined name 'add_devices'

bump geizhals version

d20403d

homeassistant added the cla-signed label Aug 24, 2018

Merge branch 'dev' into patch-2

ed1beed

balloob merged commit 2e9db1f into home-assistant:dev Aug 27, 2018

ghost removed the in progress label Aug 27, 2018

balloob mentioned this pull request Sep 17, 2018

0.78.0 #16666

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

ghost added integration: geizhals and removed platform: sensor.geizhals labels Mar 21, 2019

Uh oh!

Conversation

JulianKahnert commented Aug 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

balloob Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

JulianKahnert Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

JulianKahnert commented Aug 17, 2018

Uh oh!

MartinHjelmare commented Aug 17, 2018

Uh oh!

JulianKahnert commented Aug 17, 2018

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

JulianKahnert commented Aug 17, 2018

Uh oh!

JulianKahnert commented Aug 22, 2018

Uh oh!

MartinHjelmare commented Aug 22, 2018

Uh oh!

MartinHjelmare Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

balloob commented Aug 24, 2018

Uh oh!

JulianKahnert commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houndci-bot Aug 24, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JulianKahnert commented Aug 15, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

JulianKahnert commented Aug 24, 2018 •

edited

Loading